-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSL support #28
base: main
Are you sure you want to change the base?
SSL support #28
Conversation
To run SSL tests with CI, it is required to have a token to download Tarantool Enterprise Edition. Unfortunately, it seems that it is impossible for now due to legal reasons. We're discussing this with Product team to make it possible or provide some alternative solutions, but for now there are no results. |
Is there a way to emulate SSL support from Tarantool Enterprise for tests?
It will not be possible to test full compatibility with Tarantool Enterprise, but basic TLSv1.2 support seems to me is possible to test: |
@Mons said in Russian Tarantool Community chat discussion that it won't work: https://t.me/tarantoolru/188258 |
To be honest, I don't understand why. If we look at the code in the pull request, the binary protocol is simply wrapped by TLS. Could you try to test it (or ask @Mons for the exact reason) to make it clear? |
Yeah, I will try to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this awesome contribution! Just a few moments I think need to be addressed. And the main concern of mine are automatic tests. It is super important to be able to test automatically in the Github Actions environment this new SSL addition. It is pretty dangerous to be releasing and (which is more important) supporting in the future a functionality that cannot be tested in any way. Please suggest a viable solution to this. Look forward to hearing from you.
asynctnt/connection.py
Outdated
unix_path | ||
) | ||
unix_path, | ||
ssl=ssl_context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it will be good to allow specifying ssl_handshake_timeout as well (forward from Connection __init__
params, with the default to None). In both create_unix_connection
and create_connection
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout is supported only since 3.7. I've added a description note, but should I prove something more noticeable?
3fb0756
to
56fc62a
Compare
Yes, I agree with you. Due to a holiday season we haven't been able to discuss this matter yet, but we definitely will as soon as possible. |
56fc62a
to
81ca2dd
Compare
It seems I was wrong |
asynctnt/connection.py
Outdated
@@ -330,6 +425,8 @@ async def full_connect(): | |||
logger.debug("connect is cancelled") | |||
self._reconnect_task = None | |||
raise | |||
except ssl.SSLError as e: | |||
raise SSLError(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just noticed this thing. This is a debatable solution. Why did you decide to specifically catch SSLError and stop the reconnection process? I see a potential problem here. For example, let's say everything works fine, then Tarantool server is reconfigured but incorrectly which leads to ssl errors while connecting and connection tries to reestablish in the background. And this except block raises exception without a possibility to wait for the reconnect process giving a chance to reconfigure Tarantool once more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the idea was just to wrap all SSL-related errors to our SSLError
class. I reworked the code so ssl errors are processed as usual exceptions.
81ca2dd
to
2d966cf
Compare
Stream tests use `tarantool -V` output to check version. For Tarantool Enterprise, `tarantool -V` output is "Tarantool Enterprise <version>", thus it is incorrect to extract the version tag by splitting by spaces. Part of igorcoding#22
2d966cf
to
c10496e
Compare
This patch adds support for using SSL to encrypt the client-server communications [1]. The patch is based on a similar patch in tarantool/tarantool-python connector [2]. To use SSL encrypted connection, use Connection parameters: conn = asynctnt.Connection(host='127.0.0.1', port=3301, transport=asynctnt.Transport.SSL, ssl_key_file='./ssl/host.key', ssl_cert_file='./ssl/host.crt', ssl_ca_file='./ssl/ca.crt', ssl_ciphers='ECDHE-RSA-AES256-GCM-SHA384') If Tarantool server uses "ssl" transport, client connection also need to use asynctnt.Transport.SSL transport. If server side had ssl_ca_file set, ssl_key_file and ssl_cert_file are mandatory from the client side, otherwise optional. CA file and ciphers are optional. See available ciphers in Tarantool EE documentation [3]. 1. https://www.tarantool.io/en/enterprise_doc/security/#enterprise-iproto-encryption 2. tarantool/tarantool-python#220 3. https://www.tarantool.io/en/enterprise_doc/security/#supported-ciphers Closes igorcoding#22
SSL encrypted server could be started with Tarantool Enterprise 2.10 or newer. To configure encryption, additional listen params must be passed. ssl_key_file and ssl_cert_file are mandatory if transport is asynctnt.Transport.SSL . Follows up igorcoding#22
c10496e
to
e3de017
Compare
To run SSL tests, use Tarantool Enterprise 2.10 or newer and set TEST_TT_SSL=TRUE flag. The patch is based on a similar patch in tarantool/tarantool-python connector [1]. 1. tarantool/tarantool-python#220 Follows up igorcoding#22
e3de017
to
369d932
Compare
Fix running tests for Tarantool Enterprise
Stream tests use
tarantool -V
output to check version. For TarantoolEnterprise,
tarantool -V
output is "Tarantool Enterprise ",thus it is incorrect to extract the version tag by splitting by spaces.
Part of #22
Rename internal _transport variable
"transport" is a keyword that is used to define SSL connection in
Tarantool Enterprise 2.10 and newer. It would be convenient to use
self._transport
to store user inputtransport
variable, but thisname is already used to store connection transport. This patch renames
the instance variable.
Part of #22
Support SSL encrypted connection to Tarantool EE
This patch adds support for using SSL to encrypt the client-server
communications [1]. The patch is based on a similar patch in
tarantool/tarantool-python connector [2].
To use SSL encrypted connection, use Connection parameters:
If Tarantool server uses "ssl" transport, client connection also need to
use asynctnt.Transport.SSL transport. If server side had ssl_ca_file
set, ssl_key_file and ssl_cert_file are mandatory from the client side,
otherwise optional. CA file and ciphers are optional. See available
ciphers in Tarantool EE documentation [3].
Closes #22
Support starting Tarantool server with SSL
SSL encrypted server could be started with Tarantool Enterprise 2.10 or
newer. To configure encryption, additional listen params must be passed.
ssl_key_file and ssl_cert_file are mandatory if transport is
asynctnt.Transport.SSL .
Follows up #22
Add SSL tests
To run SSL tests, use Tarantool Enterprise 2.10 or newer and set
TEST_TT_SSL=TRUE flag. The patch is based on a similar patch in
tarantool/tarantool-python connector [1].
Follows up #22